Skip to content

Conversation

@topperc
Copy link
Collaborator

@topperc topperc commented Jun 27, 2025

When CSEing a load with an existing load with different range
metadata, clear the range metadata on the existing
load.

This is conservative, alternatively we could calculate new range
metadata using MDNode::getMostGenericRange. Without a test case I wasn't
sure it was worth it.

MDnode::getMostGenericRange takes a non-const MDNode*, but all of SelectionDAG
uses const MDNode*. A const_cast will need to be used somewhere or
we need to make the codebase consistent about whether MDNode pointers
should be const or not.

I'm sure this isn't the only place that needs to be updated to handle
the CSE.

Fixes #145363.

@llvmbot llvmbot added the llvm:SelectionDAG SelectionDAGISel as well label Jun 27, 2025
@llvmbot
Copy link
Member

llvmbot commented Jun 27, 2025

@llvm/pr-subscribers-llvm-selectiondag

Author: Craig Topper (topperc)

Changes

When CSEing a load with an existing load with different range
metadata, clear the range metadata on the existing
load.

This is conservative, alternatively we could calculate new range
metadata using MDNode::getMostGenericRange. Without a test case I wasn't
sure it was worth it.

MDnode::getMostGenericRange takes a non-const MDNode*, but all of SelectionDAG
uses const MDNode*. A const_cast will need to be used somewhere or
we need to make the codebase consistent about whether MDNode pointers
should be const or not.

I'm sure this isn't the only place that needs to be updated to handle
the CSE.

Fixes #145363.


Full diff: https://github.com/llvm/llvm-project/pull/146026.diff

3 Files Affected:

  • (modified) llvm/include/llvm/CodeGen/SelectionDAGNodes.h (+8)
  • (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp (+3)
  • (added) llvm/test/CodeGen/RISCV/pr145363.ll (+18)
diff --git a/llvm/include/llvm/CodeGen/SelectionDAGNodes.h b/llvm/include/llvm/CodeGen/SelectionDAGNodes.h
index 92da4ef7f0556..a3675eecfea3f 100644
--- a/llvm/include/llvm/CodeGen/SelectionDAGNodes.h
+++ b/llvm/include/llvm/CodeGen/SelectionDAGNodes.h
@@ -1497,6 +1497,14 @@ class MemSDNode : public SDNode {
     MMO->refineAlignment(NewMMO);
   }
 
+  void refineRanges(const MachineMemOperand *NewMMO) {
+    // If this node has range metadata that is different than NewMMO, clear the
+    // range metadata.
+    // FIXME: Union the ranges instead?
+    if (getRanges() && getRanges() != NewMMO->getRanges())
+      MMO->clearRanges();
+  }
+
   const SDValue &getChain() const { return getOperand(0); }
 
   const SDValue &getBasePtr() const {
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index 30ee6a99b9dfc..d678fd588c0bc 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -9111,6 +9111,7 @@ SDValue SelectionDAG::getAtomic(unsigned Opcode, const SDLoc &dl, EVT MemVT,
   void* IP = nullptr;
   if (SDNode *E = FindNodeOrInsertPos(ID, dl, IP)) {
     cast<AtomicSDNode>(E)->refineAlignment(MMO);
+    cast<AtomicSDNode>(E)->refineRanges(MMO);
     return SDValue(E, 0);
   }
 
@@ -9403,6 +9404,7 @@ SDValue SelectionDAG::getLoad(ISD::MemIndexedMode AM, ISD::LoadExtType ExtType,
   void *IP = nullptr;
   if (SDNode *E = FindNodeOrInsertPos(ID, dl, IP)) {
     cast<LoadSDNode>(E)->refineAlignment(MMO);
+    cast<LoadSDNode>(E)->refineRanges(MMO);
     return SDValue(E, 0);
   }
   auto *N = newSDNode<LoadSDNode>(dl.getIROrder(), dl.getDebugLoc(), VTs, AM,
@@ -9624,6 +9626,7 @@ SDValue SelectionDAG::getLoadVP(ISD::MemIndexedMode AM,
   void *IP = nullptr;
   if (SDNode *E = FindNodeOrInsertPos(ID, dl, IP)) {
     cast<VPLoadSDNode>(E)->refineAlignment(MMO);
+    cast<VPLoadSDNode>(E)->refineRanges(MMO);
     return SDValue(E, 0);
   }
   auto *N = newSDNode<VPLoadSDNode>(dl.getIROrder(), dl.getDebugLoc(), VTs, AM,
diff --git a/llvm/test/CodeGen/RISCV/pr145363.ll b/llvm/test/CodeGen/RISCV/pr145363.ll
new file mode 100644
index 0000000000000..5692b0030cc54
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/pr145363.ll
@@ -0,0 +1,18 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc < %s -mtriple=riscv64 | FileCheck %s
+
+define i32 @f(ptr %0) {
+; CHECK-LABEL: f:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    lw a0, 0(a0)
+; CHECK-NEXT:    lui a1, 294471
+; CHECK-NEXT:    addi a1, a1, 1064
+; CHECK-NEXT:    addw a0, a0, a1
+; CHECK-NEXT:    ret
+  %2 = load i32, ptr %0, align 4, !range !0
+  %3 = load i32, ptr %0, align 4
+  %4 = add i32 1206154280, %3
+  ret i32 %4
+}
+
+!0 = !{i32 1, i32 2, i32 3, i32 4}

topperc added 2 commits June 26, 2025 22:43
When CSEing a load with an existing load with different range
metadata, clear the range metadata on the existing
load.

This is conservative, alternatively we could calculate new range
metadata using getMostGenericRange. Without a test case I wasn't
sure it was worth it.

getMostGenericRange takes a non-const MDNode*, but all of SelectionDAG
uses const MDNode*. A const_cast will need to be used somewhere or
we need to make the code base consistent about whether MDNode pointers
should be const or not.

I'm sure this isn't the only place that needs to be updated to handle
the CSE.

Fixes llvm#145363.
@topperc topperc force-pushed the pr/load-range-cse branch from 380785d to eeb9aaa Compare June 27, 2025 05:47
@topperc
Copy link
Collaborator Author

topperc commented Jun 27, 2025

I added another test case and combined the range metadata in AddModifiedNodeToCSEMaps too.

@topperc topperc merged commit 9df1c81 into llvm:main Jun 27, 2025
6 checks passed
@topperc topperc deleted the pr/load-range-cse branch June 27, 2025 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

llvm:SelectionDAG SelectionDAGISel as well

Projects

None yet

Development

Successfully merging this pull request may close these issues.

riscv64 and aarch64 backends propagating range metadata too aggressively

3 participants